fix(app-shell): a failed managed-snapshot refresh is not a current record (objectui#7907) - #7937
Merged
Conversation
…cord (objectui#7907)
The tail of `onManageChanged` — the managed-snapshot refresh that runs after every
lifecycle action fired from the `PackageDetailSheet` — swallowed a failed
`fetchFullPackage` under a bare `catch {}` commented "keep the current snapshot".
That snapshot is one the action itself had just made stale, so the author disabled a
package, was told nothing, and went on reading `Status: Enabled`.
`PackageDetailSheet` derives its lifecycle verb from the record it holds (`enabled`
picks both the button label and the endpoint it POSTs), so leaving it open over a
snapshot known to be pre-action re-armed the author with the verb they had just
fired. The failure is now reported through this surface's existing objectui#7368
posture — `formatMetadataError` on the shared `studio-package-list` sonner id, so one
outage that rejects both halves of this callback is still one toast — and the sheet
closes rather than present the pre-action record as current. Still a degradation and
never a throw: the editor, the top bar and the package list stay, and no navigation
is inferred from a refresh that could not happen (objectui#7821).
The same tail dropped `fresh === null` — a successful read whose list no longer
contains the package — just as quietly, and now reports it with the sentence
`openManage` already uses.
Not recorded in `pkgsErr`, measured rather than inherited: that slot is written
exactly where `pkgs` is — the mount effect and this callback's HEAD. The tail writes
neither; it writes `manage`. The head has just recorded the list's own verdict, so
writing the slot from here would mark the trigger `failed` over names the head
refreshed successfully a moment ago.
Pre-existing, and objectui#7881 (PR #7906) made it much easier to hit rather than
causing it: before that fix this `catch` could only ever see a `res.json()`
rejection; now that `fetchFullPackage` refuses a non-2xx it also swallowed every
401 / 403 / 503 / 500.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sam
marked this pull request as ready for review
September 6, 2026 03:55
This was referenced Sep 6, 2026
os-sam
deleted the
claude/issue-7907-manage-snapshot-refresh-swallows
branch
September 6, 2026 04:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7907
The defect
The tail of
onManageChanged— the managed-snapshot refresh that runs after everylifecycle action fired from the
PackageDetailSheet(disable / enable / duplicate /publish / publish-drafts / manifest edit) — was:
The comment is true of what it did, and it is the reason it was wrong. The record in
manageis already known to be out of date at that point: the action the authorjust fired is what changed it, and this read is what replaces it. So the author disabled
a package, was told nothing, and went on reading
Status: Enabled— the pre-actionrecord presented as current.
⭐ #7881 did not introduce this — it made this arm swallow MORE
Fixing one swallowed-error defect makes another swallowing site swallow more. Before
objectui#7881 (PR #7906)
fetchFullPackagenever readres.ok, so thiscatchcouldonly ever see a
res.json()rejection — a non-JSON body. Now that the helper refuses anon-2xx, the same
catchalso swallowed every 401 / 403 / 503 / 500 the endpointserves. ⛔ Not a regression from #7906: a pre-existing defect that #7906 made much
easier to hit. A reader who meets this card without that sentence will conclude the
opposite.
The response shapes, measured on THIS arm
⛔ Not taken from #7881 on trust — re-measured at the producer, because this arm reads
fetchFullPackageand could have taken a different path. It does not: it isGET /api/v1/packages, served by the direct-mount registrar(
@objectstack/restpackage-routes.ts). Its list handler contains no hand-writtenbody — success leaves through
sendOk, every failure throughsendError/sendThrownError, andsendErrorwrites exactly{ success: false, error: { code, message, ...extra } }.UNAUTHENTICATEDFORBIDDENstudio.access/setup.accesscapability gateSERVICE_UNAVAILABLEINTERNAL_ERRORerror.codeis the only discriminating wordres.json()rejectsfetchitself rejects (offline / DNS / reset) — there is nores.okto read and no envelope to quotefresh === null; NOT a throwTwo of those are additions to #7881's table rather than restatements of it, and both are
specific to this arm: it runs immediately after a mutating request, which is when a
transport drop is most likely, and it is reached with a package id that a concurrent
delete can have removed.
The 400
VALIDATION_ERRORof the sibling/packages/:idroutes is not reachablehere: it comes from repeated query parameters, and this call sends no query string.
One shape measured and NOT covered, reported as asked. Since the framework's #12502
this door also emits
error.userMessage— text a producer marked at throw time asaddressed to the end user, which ADR-0112 tells a consumer to render verbatim.
fetchFullPackagereads onlyerror.messageanderror.code, so on a 5xx it shows thewithheld generic sentence and drops the producer's marked one. That read is
fetchFullPackage's, i.e. #7906's surface and shared by all four of its callers, notthis card's tail — filed separately rather than fixed here.
The fix
formatMetadataErroron the sharedstudio-package-listsonner id. One outagerejects both halves of this callback and both report on that id, so it stays one
toast rather than a stack. ⛔ No new state machine, no new channel.
fresh === nullis reported too — a successful read whose list no longer containsthe package was dropped by
if (fresh)just as quietly as thecatchdropped athrow. It now says so with the sentence
openManagealready uses for exactly thatfact (
engine.studio.pkg.manageMissing).Why closing, and why it is not "just close it"
PackageDetailSheetis an action surface, and it derives its verb from the record itwas handed:
enabled = pkg.enabled !== false && pkg.status !== 'disabled'picks both thebutton's label and the endpoint it POSTs (
.../${enabled ? 'disable' : 'enable'},PackagesPage.tsxlines 308 / 500 / 650). Left open over a snapshot known to bepre-action it does not merely show a stale badge — it re-arms the author with the verb
they just fired: disable succeeds, the refresh fails, the button still reads "Disable"
and still POSTs
/disable.It remains a degradation, never a throw (objectui#7368's standing ruling — one 503
must not take the Studio down): the editor, the top bar and the package list all stay,
the trigger still works, and reopening the sheet re-runs this same read one click away —
which objectui#7881 taught to report its own outcome. What is deliberately not done is
the card's option 2, marking the record stale so the staleness outlives the toast: that
needs a new prop on
PackageDetailSheet, which lives inmetadata-admin/PackagesPage.tsx— outside this card's file surface and a signature change under Clause-②. Raised in the
report rather than taken.
The
pkgsErrslot: measured on this arm, and NOT written#7881 ruled A (do not write it) from the in-file rule that
pkgsErris written exactlywhere
pkgsis, andopenManagenever writespkgs. This callback does writepkgs, so that ruling could not simply be carried over. Measured instead:pkgsis written at exactly two sites — the mount effect and the head of thiscallback — and
pkgsErris written at those same two sites and nowhere else.manage.So the relevant unit is the arm, not the callback: by the time the tail runs, the
head has already recorded the list's own verdict (a fresh list plus
setPkgsErr(null),or its failure). Writing
pkgsErrfrom the tail would mark the triggerfailedovernames the head had just refreshed successfully — objectui#7368's lie pointed the other
way. Verdict A, on this arm's own measurement, for a different reason than #7881's.
Evidence — the ablation, with predictions written first
Predictions were recorded before any leg ran. All three legs matched, in count and in
pin identity.
origin/main(f5d2acc35), pins keptfinally), no reportingLeg A's red set was §1.1 §1.2 §1.3 §2.1 §2.2 §3 §4 §5.1 §5.2 with the four §6 controls
green — exactly as predicted. The load-bearing red, verbatim:
That is the defect itself: after a lifecycle action and a 503 on the refresh, the sheet
is still on screen holding the pre-action record (
status="active",enabled="true"), andAssertionError: expected "vi.fn()" to be called at least onceon the sibling pin says nothing was reported at all.
⭐ Legs B and C are what exclude the degenerate fixes, and they fail from opposite
sides:
eight reporting pins stay red. A "fix" that merely closes cannot pass this file.
controls, because it breaks the successful refresh. §6.4 stays green because a real
deletion is decided by the head and never reaches the tail.
Together they pin the fix between the two ways of being wrong: report without breaking
the refresh, close without closing always.
The mutation was proved on disk before anything was read, and the restore by state:
86d9320ddeba94ef86d9320d,git diff HEAD --name-onlyempty1feb544986d9320d, empty8cd7eaf386d9320d, emptyAnchors flipped with it — leg A:
manageRefreshFailed1 to 0,keep the current snapshot0 to 1; legs B/C:manageRefreshFailed1 to 0 and their own marker 0 to 1.An empty or unchanged hash aborted the leg rather than letting it be read as a result.
Leg A's blob
deba94efis the same blob PR #7906 recorded as its HEAD blob, which isthe expected cross-check: leg A restores exactly the state #7906 landed.
The script carried
trap ... EXIT INT TERMrestoring an absolute path, and everyrestore named
HEADexplicitly —git checkout ref -- pathwrites the index too, so abare
git checkout -- pathwould have handed the mutation straight back whilegit statusread clean.No rebuild leg applies: the pins import
./StudioDesignSurfaceby relativespecifier, so vitest transforms the
.tsxsource and nodist/stands between themutation and the run.
Verification — all on the pushed commit
e09e9857e,git diff HEADemptyTest Files 1 passed (1)·Tests 13 passed (13)Test Files 4 passed (4)·Tests 34 passed (34)studio-design/directoryTest Files 51 passed (51)·Tests 284 passed (284)metadata-admin/suites reading the packages surface / i18n tableTest Files 4 passed (4)·Tests 18 passed (18)pnpm --filter @object-ui/app-shell run type-checknode scripts/check-changeset-presence.mjs✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/7907-manage-snapshot-refresh-reports.md.check-changeset-no-major·-fixed·-overwrite✅ No changeset declares a major bump.·✅ All workspace packages are in the changeset fixed group.·✅ No pre-existing changeset was modified or deleted.check:governed-queue-guard --test(the 3 changed paths)✅ NOT GOVERNED — 3 path(s) checked against 5 governed surface(s); none matched.check:governed-queue-guard(--self-test)OK check-governed-queue-guard self-test: 132 cases passcheck:i18n-keys·check:i18n-drift·check:i18n-dead-keyscheck:vi-mock-specifiers·check:vi-mock-inherit✅ ... OKbothcheck:control-bytes✅ check-control-bytes: OK (scanned 6402 tracked text file(s); skipped 85 binary).check:unreferenced-sourcesOK Every shipped source file in every covered package is reachable.The type-check green is not vacuous:
tsc -p tsconfig.test.json --listFilescontainsall three edited/added files, 1 hit each. It also needed the dependency closure built
first (
pnpm --filter '@object-ui/app-shell^...' build) — without it the run was 100+TS2307 Cannot find module @object-ui/..., the stale-dist trap, not a real red.The gate family was derived by hand from the changed paths against
package.jsonand.github/workflows/— this repo has noscripts/pm/dispatch-gates.mjs(that scriptlives in objectstack and answers only about its own tree).
Lint — a measured narrowing, not a skip
The repo-wide scan is CI's run. Measured here instead:
eslint .over the whole affectedpackage linted 1081 files (count from
--format json), 0 errors.i18n.ts. The 19 warnings onStudioDesignSurface.tsxare all pre-existing — they sitat lines 760-4280, and every line this PR adds is in 512-590. Notably
react-hooks/exhaustive-depsdoes not flagonManageChanged:localewas added toits dependency array with the new
tFormatreads.eslint.config.jsextendsjs.configs.recommended+tseslint.configs.recommendedand declares noparserOptions.projectand noprojectService(0 matches) — type-aware linting is notenabled, so every rule's verdict is a function of that file's own text plus the shared
config. A diff confined to three files cannot move any untouched file's verdict.
Boundaries held
/homerecovery destination is untouched — that is objectui#7373.onManageChangedis untouched (objectui#7821 / PR fix(app-shell): a failed package-list refresh is not a deletion (objectui#7821) #7879), and so isopenManage(objectui#7881 / PR fix(app-shell): a failed package lookup is not an empty result (objectui#7881) #7906). The only removed lines in the whole diff arethe four of the old tail.
of deletion; a successful list that lacks the package still belongs to the head, and
§6.4 pins that it still evicts exactly as before. Reporting is not inferring.
Generated by Claude Code